-
-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix async switches #110
Fix async switches #110
Conversation
If any switches are async, the subsequent code will execute before the switches are finished. This commit moves all that code to a new function, and debounces the calls to onSwitch() so it only executes once, after all the switches finish. Fizes #72.
@tremby What do you think? |
Will read and respond tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concept of this PR is great and it solves a couple of issues I was having that were caused by onSwitch
calling parseDOM
multiple times before I was ready (I have an async switch and I'm using a custom switch method to dynamically opt certain links out of Pjax).
In testing with my project, however, I found that there's a slight logic flaw that needs correcting, which I've commented on inline. Once that's sorted though, this looks like it'll be really useful. Nice work!
lib/switches-selectors.js
Outdated
@@ -24,6 +24,9 @@ module.exports = function(switches, switchesOptions, selectors, fromEl, toEl, op | |||
if (this.log) { | |||
this.log("newEl", newEl, "oldEl", oldEl) | |||
} | |||
|
|||
this.state.numPendingSwitches++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incrementing numPendingSwitches
in this loop doesn't work -- as the switch functions are executed in the same loop, multiple synchronous switches will result in numPendingSwitches
being incremented to 1
and then immediately decremented to 0
by the new code in onSwitch
. This then results in afterAllSwitches
being called more than once and throwing an error on the second run as at that point the state has been reset, resulting in this.state.options
being null
.
It would seem safe to instead assign numPendingSwitches
before any looping, on the very first line of the method, like so:
module.exports = function(switches, switchesOptions, selectors, fromEl, toEl, options) {
this.state.numPendingSwitches = selectors.length;
selectors.forEach(function(selector) {
// snip...
If one or more of the selectors aren't present in the switched document, the check made for each selector for newEls.length !== oldEls.length
will throw an error and result in a full page reload being triggered.
I've tested this modification with my current project that has 3 synchronous and 1 async switches and it works for me, but obviously further testing would be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it's more complicated than that, since it's not just one per selector, it's also times the number of newEls
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point 😉 You're quite right, I missed that.
Might need to loop selectors
twice then: once to build the numPendingSwitches
correctly, and once to fire off the switch callbacks. This works for me (I cache the querySelector operation results in a map to avoid unnecessary extra queries in the second loop):
var forEachEls = require("./foreach-els")
var defaultSwitches = require("./switches")
module.exports = function(switches, switchesOptions, selectors, fromEl, toEl, options) {
var elsBySelectors = {};
selectors.forEach(function(selector) {
var newEls = fromEl.querySelectorAll(selector)
var oldEls = toEl.querySelectorAll(selector)
elsBySelectors[selector] = {
newEls: newEls,
oldEls: oldEls
}
this.state.numPendingSwitches += newEls.length
}, this)
selectors.forEach(function(selector) {
var newEls = elsBySelectors[selector].newEls
var oldEls = elsBySelectors[selector].oldEls
if (this.log) {
this.log("Pjax switch", selector, newEls, oldEls)
}
if (newEls.length !== oldEls.length) {
// forEachEls(newEls, function(el) {
// this.log("newEl", el, el.outerHTML)
// }, this)
// forEachEls(oldEls, function(el) {
// this.log("oldEl", el, el.outerHTML)
// }, this)
throw "DOM doesn’t look the same on new loaded page: ’" + selector + "’ - new " + newEls.length + ", old " + oldEls.length
}
forEachEls(newEls, function(newEl, i) {
var oldEl = oldEls[i]
if (this.log) {
this.log("newEl", newEl, "oldEl", oldEl)
}
if (switches[selector]) {
switches[selector].bind(this)(oldEl, newEl, options, switchesOptions[selector])
}
else {
defaultSwitches.outerHTML.bind(this)(oldEl, newEl, options)
}
}, this)
}, this)
}
Alternatively, you could stick to one loop of selectors
and build a queue of switch callbacks to execute after the loop is done and numPendingSwitches
has been calculated.
See what you think of either of those approaches...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the queue approach is more elegant:
var forEachEls = require("./foreach-els")
var defaultSwitches = require("./switches")
module.exports = function(switches, switchesOptions, selectors, fromEl, toEl, options) {
var queue = [];
selectors.forEach(function(selector) {
var newEls = fromEl.querySelectorAll(selector)
var oldEls = toEl.querySelectorAll(selector)
if (this.log) {
this.log("Pjax switch", selector, newEls, oldEls)
}
if (newEls.length !== oldEls.length) {
// forEachEls(newEls, function(el) {
// this.log("newEl", el, el.outerHTML)
// }, this)
// forEachEls(oldEls, function(el) {
// this.log("oldEl", el, el.outerHTML)
// }, this)
throw "DOM doesn’t look the same on new loaded page: ’" + selector + "’ - new " + newEls.length + ", old " + oldEls.length
}
this.state.numPendingSwitches += newEls.length
forEachEls(newEls, function(newEl, i) {
var oldEl = oldEls[i]
if (this.log) {
this.log("newEl", newEl, "oldEl", oldEl)
}
var callback = (switches[selector]) ?
switches[selector].bind(this, oldEl, newEl, options, switchesOptions[selector]) :
defaultSwitches.outerHTML.bind(this, oldEl, newEl, options)
queue.push(callback)
}, this)
}, this)
queue.forEach(function(callback) {
callback()
})
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better, we don't need to increment this.state.numPendingSwitches
manually. Instead, once all the switches are queued up, we can just do this.state.numPendingSwitches = queue.length
.
index.js
Outdated
href) | ||
} | ||
this.state.options.title, | ||
this.state.href) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this method refers to this.state
a lot, you could do the following for brevity and readability:
var state = this.state;
window.history.pushState({
url: state.href,
title: state.options.title,
uid: this.maxUid
},
state.options.title,
state.href)
This is just personal preference, but I find it makes things a bit easier to parse visually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
With a lot of help from @robinnorth
@MoOx Can you add @robinnorth as a collaborator? |
I'm not sure I can help. The PR description sounds valuable, but the changeset is too broad for my knowledge of the internals of this project. Bowing out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
index.js
Outdated
@@ -181,6 +174,8 @@ Pjax.prototype = { | |||
else if (request.getResponseHeader("X-XHR-Redirected-To")) { | |||
href = request.getResponseHeader("X-XHR-Redirected-To") | |||
} | |||
this.state.href = href | |||
this.state.options = options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid mutation if possible and prefer a new object here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mutation of what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.state. Imo it's better to avoid a mutation and create a new state (in case you want to compare a state change, it's easier to do state === prevState)
I am mostly doing react this days, and I was thinking about this https://reactjs.org/docs/react-component.html#shouldcomponentupdate :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure what you mean. state
is just a name I picked for the property to store the current href
and options
, so we can get it after the switches call back. After that, it gets destroyed (here). We could have used this.currentHref
for the same results.
We could pass them around instead of making it a global property, but that gets messy and confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you mean we should do
this.state = {
href: href,
options: options
}
instead of
this.state.href = href
this.state.options = options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I mean. But you will probably need to copy the other state key/value.
Anyway, if you feel it's totally useless, feel free to move forward and ignore my comment. I am just quickly reviewing here as I currently don't use this library on any active project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason to need the state once we're done with it, or to keep track of previous states, so I'm going to leave it for now.
@BehindTheMath added! |
Thanks @BehindTheMath, @MoOx! I've added one small extra commit that hopefully takes care of your comment regarding state mutation, @MoOx. Everything looks good to me, so I think you're good to merge, @BehindTheMath 👍 |
@robinnorth Thanks for all the comments and help. |
If any switches are async, the subsequent code will execute before the switches are finished. This commit moves all that code to a new function, and debounces the calls to onSwitch() so it only executes once, after all the switches finish.
Fizes #72.